fix HIL-SERL keyboard teleop #2345
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in keyboard event handling and moves the current_pressed state clearing to the correct location. The key changes ensure that special keys (like ESC) are properly handled and that the pressed key state is maintained correctly between method calls.
- Fixed handling of special keys (non-character keys) in
_on_pressand_on_releasemethods - Moved
current_pressed.clear()fromget_action()toget_teleop_events()to preserve state across method calls
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key = key.char | ||
| self.event_queue.put((key, False)) | ||
|
|
||
| if key == keyboard.Key.esc: |
There was a problem hiding this comment.
After the change at lines 110-111, the key variable is reassigned to key.char if it has a 'char' attribute. This will cause the ESC key comparison to fail because keyboard.Key.esc does not have a 'char' attribute, so key will still be keyboard.Key.esc at this check. However, this means the comparison will work correctly. But the logic is inconsistent with lines 104-107 in _on_press. Consider checking the original key value before reassignment, or use a separate variable to avoid confusion.
There was a problem hiding this comment.
in linux spacial key do not have char, but don't know if this change affect other part of LeRobot
| def _on_press(self, key): | ||
| if hasattr(key, "char"): | ||
| self.event_queue.put((key.char, True)) | ||
| key = key.char | ||
| self.event_queue.put((key, True)) |
There was a problem hiding this comment.
The change now allows special keys (like arrow keys, shift, ctrl) to be added to event_queue as keyboard.Key objects when they don't have a 'char' attribute. However, in get_action() (line 196), the code directly compares keys with keyboard.Key objects (e.g., key == keyboard.Key.up), which will work. But in the base KeyboardTeleop.get_action() (line 137), the code creates a set from current_pressed items without type awareness. This inconsistency could lead to mixed key types in current_pressed dictionary, which may cause unexpected behavior when comparing or processing keys.
There was a problem hiding this comment.
in linux spacial key do not have char, but don't know if this change affect other part of LeRobot
1 participant
What this does
fix KeyboardEndEffectorTeleop for HIL-SERL
Examples:
How it was tested
How to checkout & try? (for the reviewer)
{ "env": { "robot": { "type": "so100_follower", "port": "/dev/usb_follower_arm", "use_degrees": true, "id": "main_follower", "cameras": { "top": { "type": "opencv", "index_or_path": "/dev/usb_realsense_rgb", "height": 240, "width": 320, "fps": 30, "rotation": 180 }, "wrist": { "type": "opencv", "index_or_path": "/dev/usb_follower_cam_wrist", "height": 240, "width": 320, "fps": 30 } } }, "teleop": { "type": "keyboard_ee", "use_gripper": true }, "processor": { "control_mode": "keyboard_ee", "observation": { "display_cameras": false, "add_joint_velocity_to_observation": true, "add_current_to_observation": true }, "image_preprocessing": { "crop_params_dict": null, "resize_size": [ 128, 128 ] }, "gripper": { "use_gripper": true, "gripper_penalty": -0.02 }, "reset": { "fixed_reset_joint_positions": [ 0.00, -20.00, 25.00, 90.00, 90.00, 30.00 ], "reset_time_s": 2.5, "control_time_s": 20.0, "terminate_on_success": true }, "inverse_kinematics": { "urdf_path": "../SO-ARM100/Simulation/SO101/so101_new_calib.urdf", "target_frame_name": "gripper_frame_link", "end_effector_bounds": { "min": [ 0.115, -0.145, -0.0075 ], "max": [ 0.26, 0.145, 0.1 ] }, "end_effector_step_sizes": { "x": 0.005, "y": 0.005, "z": 0.01 } }, "reward_classifier": { "pretrained_path": null, "success_threshold": 0.5, "success_reward": 1.0 }, "max_gripper_pos": 30 }, "name": "real_robot", "fps": 10 }, "dataset": { "repo_id": "jpizarrom/pick_lift_cube_pipeline_test", "root": null, "task": "", "num_episodes_to_record": 2, "replay_episode": null, "push_to_hub": false }, "mode": null, "device": "cuda" }